Make ReleaseFile TTI by tracking its last access time
#95867
Conversation
This brings the legacy `ReleaseFile`s in line with how debug files and `ArtifactBundle`s are being expired. My hope was to just be able to completely deprecate and delete all of `ReleaseFile`, but even though the usage of these is well below <1%, the deprecation isn’t really going anywhere. So might as well just give these a access-time-based TTI and move on. Most of the code is just a straight up copy of how this was done for debug files, except for the actual deletion job, which still needs to be implemented.
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This PR has a migration; here is the generated SQL for for --
-- Add field date_accessed to releasefile
--
ALTER TABLE "sentry_releasefile" ADD COLUMN "date_accessed" timestamp with time zone DEFAULT (STATEMENT_TIMESTAMP()) NOT NULL; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #95867 +/- ##
==========================================
- Coverage 87.74% 79.25% -8.50%
==========================================
Files 10582 10583 +1
Lines 610032 610124 +92
Branches 23976 23976
==========================================
- Hits 535276 483548 -51728
- Misses 74472 126292 +51820
Partials 284 284 |
There was a problem hiding this comment.
Bug: Incorrect Metadata Update Affects File Cleanup
The ReleaseFile.update() method incorrectly updates the date_accessed field when the name is changed and ident is not provided. This field is intended for TTI (Time To Idle) to track read access, not metadata modifications. This behavior incorrectly resets the TTI timer, preventing proper cleanup of files that have not been genuinely accessed.
src/sentry/models/releasefile.py#L117-L118
sentry/src/sentry/models/releasefile.py
Lines 117 to 118 in 8d27d60
Bug: Inconsistent Timestamps in File Renewal
A race condition exists due to inconsistent now and threshold_date calculations. The maybe_renew_releasefiles function identifies files for renewal based on a computed threshold_date, but renew_releasefiles_by_id recomputes these values independently. This breaks the intended timestamp consistency, potentially causing files selected for renewal to not be updated if the threshold_date changes between the two function calls.
src/sentry/debug_files/release_files.py#L15-L32
sentry/src/sentry/debug_files/release_files.py
Lines 15 to 32 in 8d27d60
Was this report helpful? Give feedback by reacting with 👍 or 👎
This brings the legacy `ReleaseFile`s in line with how debug files and `ArtifactBundle`s are being expired. My hope was to just be able to completely deprecate and delete all of `ReleaseFile`, but even though the usage of these is well below <1%, the deprecation isn’t really going anywhere. So might as well just give these a access-time-based TTI and move on. Most of the code is just a straight up copy of how this was done for debug files (#52257) **over 2 years ago**, except for the actual deletion job, which still needs to be implemented.
This brings the legacy
ReleaseFiles in line with how debug files andArtifactBundles are being expired.My hope was to just be able to completely deprecate and delete all of
ReleaseFile, but even though the usage of these is well below <1%, the deprecation isn’t really going anywhere.So might as well just give these a access-time-based TTI and move on.
Most of the code is just a straight up copy of how this was done for debug files (#52257) over 2 years ago, except for the actual deletion job, which still needs to be implemented.